-
Notifications
You must be signed in to change notification settings - Fork 14
fix: move restart on error Actor option to Run options #508
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't someone from tooling team who handles Python development be assigned for review too, instead of 3 platform devs?
Yep, added @janbuchar. |
Shouldn't this be a |
max_items: int | None = None, | ||
memory_mbytes: int | None = None, | ||
timeout_secs: int | None = None, | ||
restart_on_error: bool | None = None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: @janbuchar, is this a breaking change (changing the order of the parameters) ? I'm not sure about the exact intricacies of the Python argument ordering.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, technically it is. But:
- Nobody in their right mind would call this with positional (non-keyword) arguments. In fact, the entire signature of the function should probably be keyword-only, but that's breaking
- While it is not de facto private, it probably should be
That considered, is it a problem to just move the parameter down?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I wasn't specific enough and didn't choose the best example. Similar changes are present throughout this and other files in public function. Please, see the last commit.
See for instance start
. Same question there.
The positioning can be changed, but I would rather keep it near other similar options.
What do you think ? In other languages, it would definitely be a BC, but I'm not sure about the Python.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, if there's no asterisk in the list of args, I'd just add a new argument at the end, just to be safe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. Only such occurrence was in get_task_representation
in the task.py
.
Moved to the end of args list.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Im getting FBT001 Boolean-typed positional argument in function definition
linting error.
Would adding asterix in the list help?
def get_task_representation(
actor_id: str | None = None,
name: str | None = None,
task_input: dict | None = None,
build: str | None = None,
max_items: int | None = None,
memory_mbytes: int | None = None,
timeout_secs: int | None = None,
title: str | None = None,
actor_standby_desired_requests_per_actor_run: int | None = None,
actor_standby_max_requests_per_actor_run: int | None = None,
actor_standby_idle_timeout_secs: int | None = None,
actor_standby_build: str | None = None,
actor_standby_memory_mbytes: int | None = None,
*,
restart_on_error: bool | None = None,
) -> dict:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, Asterix should be able to help. The lint rule exists because free-floating boolean arguments are confusing (foo(True, True, False, True)
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
Changes related to move of
restartOnError
from Actor's top level to its run options.Core: https://github.com/apify/apify-core/pull/23381
Worker: https://github.com/apify/apify-worker/pull/1515
Shared: apify/apify-shared-js#546
JS Client: apify/apify-client-js#760
Docs: apify/apify-docs#1965